Move existing Temporal tests from tc39/proposal-temporal #3223
Conversation
Instead of an in-tree checkout use a git submodule of test262 so we can pin the tests to a particular commit and pull in updates when we are ready for them. Requires some changes in various paths. (NOTE: This is currently pinned to ptomato/test262's temporal-tests branch. Needs to be updated to point to tc39/test262's main branch once tc39/test262#3223 is merged.)
Instead of an in-tree checkout use a git submodule of test262 so we can pin the tests to a particular commit and pull in updates when we are ready for them. Requires some changes in various paths. (NOTE: This is currently pinned to ptomato/test262's temporal-tests branch. Needs to be updated to point to tc39/test262's main branch once tc39/test262#3223 is merged.)
ptomato
force-pushed
the
temporal-tests
branch
from
af1761d
to
7e9aa5f
Compare
|
@ptomato just a heads up that including |
There was a problem hiding this comment.
@ptomato I removed the console.warns from the polyfill and ran all of these against that in JSC. The following tests fail in both strict and default execution. Can you either fix the failures or provide clarity (ie. known failure for whatever reason)? Thanks!
FAIL test/built-ins/Temporal/Calendar/prototype/fields/long-input.js
Expected no error, got RangeError: invalid field name garbage 1
FAIL test/built-ins/Temporal/Duration/compare/calendar-dateadd-called-with-options-undefined.js
dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true
FAIL test/built-ins/Temporal/Duration/compare/relativeto-string-datetime.js
date-time + IANA annotation is a zoned relativeTo Expected SameValue(«-1», «0») to be true
FAIL test/built-ins/Temporal/Duration/prototype/round/calendar-dateadd-called-with-options-undefined.js
dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true
FAIL test/built-ins/Temporal/Duration/prototype/total/calendar-dateadd-called-with-options-undefined.js
dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true
FAIL test/built-ins/Temporal/ZonedDateTime/prototype/since/calendar-dateadd-called-with-options-undefined.js
dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true
FAIL test/built-ins/Temporal/ZonedDateTime/prototype/toLocaleString/options-conflict.js
Expected no error, got TypeError: dateStyle and timeStyle may not be used with other DateTimeFormat options
FAIL test/built-ins/Temporal/ZonedDateTime/prototype/until/calendar-dateadd-called-with-options-undefined.js
Expected no error, got RangeError: mixed-sign values not allowed as duration fields
|
@rwaldron Philip would know better, but I believe the project's |
|
@rwaldron Thanks for the quick review! For the console.warn, the way we run the tests against the polyfill is Here's what's going on with each of those failures:
|
This copies over the tests that previously existed in the tc39/proposal-temporal repository. For context, see thread starting at: tc39#3002 (comment) In service of tc39#3002
There were a few tests already in the tree that overlapped ones that I added in the previous commit. I've consolidated these and taken information from the deleted ones where applicable, and improved on the autogenerated assertion messages.
ptomato
force-pushed
the
temporal-tests
branch
from
7e9aa5f
to
0ca2d47
Compare
|
@rwaldron I've removed the tests that were skipped in the proposal repo now. That leaves two failures; one addressed by the existing PR. The other I've tracked down to a difference in JSC and v8: > new Intl.DateTimeFormat('en', {dateStyle:'short'}).resolvedOptions().year
undefinedJSC (as of a recent WebKit build): > new Intl.DateTimeFormat('en', {dateStyle:'short'}).resolvedOptions().year
"2-digit"It looks like JSC is not correctly implementing step 5.d of Intl.DateTimeFormat.prototype.resolvedOptions, which is causing a problem with the polyfill, but I believe this remaining test is correct as-is. |
|
@ptomato thank you—that satisfies my review! Marking R+, with the understanding that we have a lot of work to do ahead of us, and getting this in sooner is the pragmatic thing to do 👍 |
|
The CI failure is unrelated to these changes. |
|
@rwaldron Thanks for the quick review, the help to get this in quickly is much appreciated. I think the next steps for me and @Ms2ger will be to continue to port coverage over from the proposal's legacy test suite (files such as https://github.com/tc39/proposal-temporal/blob/main/polyfill/test/instant.mjs) and then continue with the checklist in #3002. If you have questions later about any of these tests, or find something that you wish you'd caught in the review, let me know and I will try to help. I realize this form of PR wasn't ideal for you. I've reported the JSC bug as https://bugs.webkit.org/show_bug.cgi?id=231041 |
As discussed in #3002, this moves all the in-progress Temporal tests from tc39/proposal-temporal into here, so that it's more accessible to implementors. Further development is intended to happen here, as these tests will be removed from tc39/proposal-temporal (though they will still be executed using a git submodule).